Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Re-implement unread counts #7736

Merged
merged 47 commits into from
Jul 29, 2020
Merged

Re-implement unread counts #7736

merged 47 commits into from
Jul 29, 2020

Conversation

babolivier
Copy link
Contributor

@babolivier babolivier commented Jun 23, 2020

Second attempt at implementing unread counts, this time without relying on push rules. This is also backing out the MSC2625 implementation, which we likely won't end up using reverting the MSC2625 implem has been moved to #7761.

Best reviewed commit by commit.

Fixes #7741

@richvdh
Copy link
Member

richvdh commented Jun 24, 2020

This is also backing out the MSC2625 implementation

which, for linking, was #7673.

@babolivier babolivier force-pushed the babolivier/unread_counts branch 2 times, most recently from 62e529d to c1b148c Compare June 26, 2020 18:37
@babolivier babolivier requested a review from a team June 26, 2020 18:40
@babolivier babolivier marked this pull request as ready for review June 26, 2020 18:40
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shaping up, thanks.

Remember that the new table will need clearing out whenever events are purged.

synapse/storage/data_stores/main/events.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/events.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/events.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/events.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/events.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/events_worker.py Outdated Show resolved Hide resolved
synapse/storage/data_stores/main/events_worker.py Outdated Show resolved Hide resolved
richvdh
richvdh previously requested changes Jul 3, 2020
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually:

I'm generally concerned about the size of the new data, and the fact that we can never clear it out.

What if we instead added a new column to the events table which indicated if it should count towards unread count. Then calculating unread count for a given user is simply SELECT count(*) from events where room_id = ? and stream_ordering > ? and sender != ? and count_as_unread.

@babolivier babolivier requested a review from a team July 27, 2020 17:48
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, but one remaining problem.

According to https://www.postgresql.org/docs/10/sql-altertable.html:

Adding a column with a DEFAULT clause or changing the type of an existing column will require the entire table and its indexes to be rewritten.

Postgres has improved this in v11, but we're still on v10 on matrix.org, and we do not want to be rewriting the events table. In any case, we need to consider other HS owners. We claim to support postgres 9.5 or later.

In short, we'll need to make the new column nullable and remove the default - and then behave sensibly when we get a NULL value back.

Unusually, sqlite is better in this area :)

@babolivier
Copy link
Contributor Author

Oooh good call, I didn't think of that, thanks! Looks like both PostgreSQL and SQLite consider NULL as a false value, so simply removing the default Just Works™.

So we don't end up rewriting the whole events table when running postgres < 11.
@babolivier babolivier requested a review from a team July 29, 2020 10:04
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@babolivier babolivier merged commit 8dff4a1 into develop Jul 29, 2020
@babolivier babolivier deleted the babolivier/unread_counts branch July 29, 2020 17:26
babolivier added a commit that referenced this pull request Jul 29, 2020
anoadragon453 added a commit that referenced this pull request Jul 30, 2020
…bership_join_count

* 'develop' of github.com:matrix-org/synapse:
  Update workers docs (#7990)
  Fix invite rejection when we have no forward-extremeties (#7980)
  Fix typo in docs/workers.md (#7992)
  Convert federation client to async/await. (#7975)
  Convert appservice to async. (#7973)
  Convert some of the data store to async. (#7976)
  Fix formatting of changelog and upgrade notes
  Ensure that remove_pusher is always async (#7981)
  Add deprecation warnings
  1.18.0
  Update worker docs with recent enhancements  (#7969)
  Ensure the msg property of HttpResponseException is a string. (#7979)
  Remove from the event_relations table when purging historical events. (#7978)
  Add additional logging for SAML sessions. (#7971)
  Add MSC reference to changelog for #7736
  Re-implement unread counts (#7736)
  Various improvements to the docs (#7899)
babolivier added a commit that referenced this pull request Aug 6, 2020
babolivier added a commit that referenced this pull request Aug 6, 2020
@babolivier babolivier mentioned this pull request Aug 6, 2020
babolivier added a commit that referenced this pull request Aug 6, 2020
babolivier added a commit that referenced this pull request Aug 11, 2020
The test that's being added here is the exact same one as the one introduced in #7736
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '3950ae51e':
  Ensure that remove_pusher is always async (#7981)
  Ensure the msg property of HttpResponseException is a string. (#7979)
  Remove from the event_relations table when purging historical events. (#7978)
  Add additional logging for SAML sessions. (#7971)
  Add MSC reference to changelog for #7736
  Re-implement unread counts (#7736)
  Various improvements to the docs (#7899)
  Convert storage layer to async/await. (#7963)
  Add an option to disable purge in delete room admin API (#7964)
  Move some log lines from default logger to sql/transaction loggers (#7952)
  Use the JSON module from the std library instead of simplejson. (#7936)
  Fix exit code for `check_line_terminators.sh` (#7970)
  Option to allow server admins to join complex rooms (#7902)
  Fix typo in metrics docs (#7966)
  Add script for finding files with unix line terminators (#7965)
  Convert the remaining media repo code to async / await. (#7947)
  Convert a synapse.events to async/await. (#7949)
  Convert groups and visibility code to async / await. (#7951)
  Convert push to async/await. (#7948)
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@richvdh richvdh mentioned this pull request Apr 5, 2022
2 tasks
@richvdh
Copy link
Member

richvdh commented Apr 5, 2022

Note that this PR was later backed out in #8039, and re-done in #8059.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement MSC2654 (unread counts)
3 participants